bugfix: Save victory status to prevent early exits from resulting in defeat in network matches#2292
bugfix: Save victory status to prevent early exits from resulting in defeat in network matches#2292Stubbjax wants to merge 9 commits intoTheSuperHackers:mainfrom
Conversation
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp | Added m_isVictorious array to cache victory status, refactored alliance checking; edge case bug remains where simultaneous elimination could leave players in inconsistent state |
| GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp | Same changes as Generals version but with indentation inconsistency (spaces instead of tabs) on line 97; same edge case bug exists |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[VictoryConditions::update called] --> B{Is multiplayer?}
B -->|No| Z[Return]
B -->|Yes| C{Single alliance remaining?}
C -->|Yes| G[Check for player eliminations]
C -->|No| D[multipleAlliancesExist?]
D -->|Yes: Multiple teams alive| Z
D -->|No: Single team or none| E[Set m_singleAllianceRemaining = true<br/>Save m_endFrame]
E --> F[findFirstUndefeatedPlayer]
F -->|Player found| H[markAllianceVictorious<br/>Set m_isVictorious for alliance]
F -->|nullptr: All defeated| G
H --> G
G --> I{For each player}
I --> J{Player defeated?}
J -->|Yes| K[Set m_isDefeated<br/>Reveal map<br/>Kill player]
J -->|No| I
K --> I
I -->|Done| L[Check local player defeat]
L --> Z
style H fill:#90EE90
style K fill:#FFB6C1
style E fill:#FFD700
Last reviewed commit: db4f4c7
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
Additional Comments (2)
The old logic returned true for any player when Add fallback logic: Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}Prompt To Fix With AIThis is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 287:302
Comment:
logic issue: losing players who quit early without asset destruction won't be marked as defeated
The old logic returned true for any player when `m_singleAllianceRemaining && !hasAchievedVictory(player)`. The new logic only checks `m_isDefeated[i]`, which is only set when `hasSinglePlayerBeenDefeated()` returns true (assets destroyed). If a losing player quits early and their assets are maintained by allies, they won't be marked defeated.
Add fallback logic:
```cpp
Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise.
The old logic returned true for any player when Add fallback logic: Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}Prompt To Fix With AIThis is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 289:304
Comment:
logic issue: losing players who quit early without asset destruction won't be marked as defeated
The old logic returned true for any player when `m_singleAllianceRemaining && !hasAchievedVictory(player)`. The new logic only checks `m_isDefeated[i]`, which is only set when `hasSinglePlayerBeenDefeated()` returns true (assets destroyed). If a losing player quits early and their assets are maintained by allies, they won't be marked defeated.
Add fallback logic:
```cpp
Bool VictoryConditions::hasBeenDefeated(Player *player)
{
if (!player)
return false;
if (!m_singleAllianceRemaining)
return false;
for (Int i = 0; i < MAX_PLAYER_COUNT; ++i)
{
if (player == m_players[i])
{
if (m_isDefeated[i])
return true;
if (!m_isVictorious[i])
return true;
return false;
}
}
return false;
}
```
How can I resolve this? If you propose a fix, please make it concise. |
…defeat in network matches
Stubbjax
force-pushed
the
fix-victory-conditions
branch
from
e688101 to
87a75a4
Compare
| Bool VictoryConditions::multipleAlliancesExist() | ||
| { | ||
| Player* alive = nullptr; |
There was a problem hiding this comment.
Incorrect win detection
multipleAlliancesExist() returns false when zero non-defeated players exist (e.g., simultaneous elimination / edge cases where everyone is defeated). In that case update() will set m_singleAllianceRemaining = true and cache a “victorious” player based on the first non-defeated player found, but there won’t be one, so no one gets marked victorious. This makes hasAchievedVictory() always return false, and can also prevent hasBeenDefeated() from ever returning true (since it now only consults m_isDefeated[i], not “not victorious”), leaving the match outcome inconsistent.
This function needs to distinguish “exactly one alliance alive” from “nobody alive” (and handle the latter explicitly).
Prompt To Fix With AI
This is a comment left during a code review.
Path: Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 146:148
Comment:
**Incorrect win detection**
`multipleAlliancesExist()` returns `false` when *zero* non-defeated players exist (e.g., simultaneous elimination / edge cases where everyone is defeated). In that case `update()` will set `m_singleAllianceRemaining = true` and cache a “victorious” player based on the first non-defeated player found, but there won’t be one, so no one gets marked victorious. This makes `hasAchievedVictory()` always return false, and can also prevent `hasBeenDefeated()` from ever returning true (since it now only consults `m_isDefeated[i]`, not “not victorious”), leaving the match outcome inconsistent.
This function needs to distinguish “exactly one alliance alive” from “nobody alive” (and handle the latter explicitly).
How can I resolve this? If you propose a fix, please make it concise.
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Outdated
Show resolved
Hide resolved
| void VictoryConditions::markAllianceVictorious(Player* victoriousPlayer) | ||
| { | ||
| // This marks the player and any allies as victorious, including defeated allies. | ||
| // This also ensures players retain their victorious status if their assets are destroyed after |
There was a problem hiding this comment.
You can add the TheSuperHackers @bugfix here and remove the other @bugfix comment, then move this comment above the function to get it a bit out of the way.
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| Player* VictoryConditions::findFirstVictoriousPlayer() |
There was a problem hiding this comment.
The function name is a bit misleading. It only returns the first victorious player if multipleAlliancesExist returns false. Otherwise it returns the first non defeated player.
There was a problem hiding this comment.
Perhaps a better name would be findFirstUndefeatedPlayer or findFirstAlivePlayer. The function does not consider multipleAlliancesExist.
| } | ||
|
|
||
| //------------------------------------------------------------------------------------------------- | ||
| Player* VictoryConditions::findFirstVictoriousPlayer() |
There was a problem hiding this comment.
Conceptually, bool multipleAlliancesExist + Player* findFirstNotDefeatedPlayer + markAllianceVictorious(Player*) is a bit cumbersome logic.
A better logic would be Player* getSingleAllianceSurvived + markAllianceVictorious(Player*)
.
There was a problem hiding this comment.
Checking for a single surviving alliance does not conceptually work either - we still want the condition to be satisfied if there are no alliances remaining.
| virtual UnsignedInt getEndFrame( void ) { return m_endFrame; } ///< on which frame was the game effectively over? | ||
| private: | ||
| Player* findFirstUndefeatedPlayer(); ///< Find the first player that has not been defeated. | ||
| void markAllianceVictorious(Player* victoriousPlayer); ///< Mark the victorious player and his allies as victorious. |
There was a problem hiding this comment.
uses spaces instead of tabs for indentation
| void markAllianceVictorious(Player* victoriousPlayer); ///< Mark the victorious player and his allies as victorious. | |
| void markAllianceVictorious(Player* victoriousPlayer); ///< Mark the victorious player and his allies as victorious. |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/VictoryConditions.cpp
Line: 97
Comment:
uses spaces instead of tabs for indentation
```suggestion
void markAllianceVictorious(Player* victoriousPlayer); ///< Mark the victorious player and his allies as victorious.
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.2 participants
This change fixes an issue where a player's victory is not reliably recorded at the end of a network match.
If a player quits the game at any point before the score screen, and it results in the destruction of their assets (i.e. no allies exist), then a loss is recorded, regardless of the actual outcome. This is because the victory condition is processed at the time the game ends and the score screen is shown. To resolve this, the player's victory is now saved as soon as one team remains and the match is considered over, and so any subsequent defeat + asset destruction has no effect on the recorded outcome.
The change includes a minor refactor by abstracting some of the update logic into a
multipleAlliancesExistfunction, and consolidating some stat conditions. There are further refactors that can be done in follow-up changes, such as readingm_isDefeatedinstead of repeatedly callinghasSinglePlayerBeenDefeatedin multiple places, and splitting several blocks in the update method into functions.Tests
Below is a series of network match test results to verify the fix.
Test 1
Knife surrenders at the start of a 1v1 match. Fork then quits the game.
Test 2
Knife surrenders at the start of a 1v1v1 match. Fork then surrenders. Spoon then quits the game.
Test 3
Knife surrenders at the start of a 2v1 match. Fork then surrenders. Spoon then quits the game.
Test 4
Fork surrenders at the start of a 2v1 match. Spoon then quits the game. Knife then quits the game.
Test 5
Knife surrenders at the start of a 2v1 match. Spoon then surrenders. Fork then quits the game.